-
Notifications
You must be signed in to change notification settings - Fork 0
Further testing #85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Further testing #85
Conversation
src/kabr_tools/cvat2slowfast.py
Outdated
| video_id = 1 | ||
| folder_name = 1 | ||
| flag = False | ||
| flag = not no_images |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional suggestion:
flag was previously set to False. It now represents the negation of the no_images argument:
- When
no_imagesisTrue,flagisFalse. - When
no_imagesisFalse,flagisTrue.
For clarity purpose, consider directly using the new argument no_images instead of introducing the flag variable, as flag is vague and less descriptive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we put this in a new PR? I agree with this suggestion, but the diff in this one is already huge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing!
tests/utils.py
Outdated
| annotation = get_cached_datafile(DETECTION_ANNOTATION) | ||
| return video, annotation | ||
|
|
||
| def clean_dir(path): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming the goal for clean_dir is to remove a directory regardless of its contents, you should use shutil.rmtree
(I made this mistake previously...)
import shutil
import os
def clean_dir(path):
if os.path.exists(path):
shutil.rmtree(path)
os.removedirsis designed to remove a directory and all of its parent directories only if they are empty. If the directory path contains files or subdirectories, this function will fail with an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need some time to re-read how this is used. I might have had some reason for picking os.removedirs over shutil.rmtree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Again sorry for the late response on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, it's for a weird case in which I want to remove the empty parent directories. The slowfast model always creates an empty, nested output directory even though it doesn't output anything with the kabr inference config. I just added this function to avoid having random empty directories locally when testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries; thanks for the review! Do you have an improved name suggestion for this function? Maybe clean_empty_dirs? I can update & rebase on main after work today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah clean_empty_dirs looks good.
Check whether a directory is empty if possible, something like:
len(os.listdir(path)) == 0And please summarize your last comment into docstrings or comment alongside the function. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebasing the 111 commits is going to take more time than what I have tonight. I'll try to get it by next Monday!
Edit: I'll also apply both requested changes to cut down on the number of PRs & reviews :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ended up running git reset --soft HEAD~111 && git commit -m "Add further testing" and git rebase origin/master. Unsure if that + manually verifying conflicts was the best approach. Waiting to see if tests pass 🎉
f56b4cd to
af6eeb8
Compare
Co-authored-by: Net Zhang
Resolves #74